-
Notifications
You must be signed in to change notification settings - Fork 25
feat: simpler component configuration #279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR refactors component plugin configuration by introducing a helper to resolve plugins by dotted path or pool name and moving allowed_plugin_types generation into setup; it enhances slot plugin registration with conflict checks, standardizes module paths for dynamically created plugins, and updates settings and tests to support name-based plugin lookup. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 88.68% 88.65% -0.03%
==========================================
Files 124 124
Lines 3366 3376 +10
Branches 283 287 +4
==========================================
+ Hits 2985 2993 +8
- Misses 264 265 +1
- Partials 117 118 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fsbraun - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Guard against empty allowed_plugin_types to avoid TypeError (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 5 other issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
djangocms_frontend/cms_plugins.py
Outdated
@@ -11,7 +12,7 @@ def update_plugin_pool(): | |||
from .component_pool import components | |||
|
|||
# Loop through the values in the components' registry | |||
for _, plugin, slot_plugins in components._registry.values(): | |||
for key, (_, plugin, slot_plugins) in components._registry.items(): | |||
if plugin.__name__ not in plugin_pool.plugins: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): We've found these issues:
- Swap if/else branches (
swap-if-else-branches
) - Remove unnecessary else after guard condition (
remove-unnecessary-else
)
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fsbraun - I've reviewed your changes - here's some feedback:
- In update_plugin_pool, you have a redundant nested for slot_plugin in slot_plugins loop—remove the inner loop to avoid double iteration.
- Overriding the plugin class’s module to a constant string may break introspection or pickling—consider keeping the original module or documenting this change.
- get_plugin_class silently fails if plugin_pool.get_plugin can’t find a name—consider raising a clear exception or logging a descriptive error.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
plugin_pool.register_plugin(slot_plugin) | ||
# Register slot plugins, checking for name conflicts | ||
for slot_plugin in slot_plugins: | ||
if slot_plugin.__name__ not in plugin_pool.plugins: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Conflict check using class name may not align with plugin_pool API
Confirm which key plugin_pool.plugins uses (e.g., plugin_name) and use that instead of name to detect conflicts accurately.
@@ -174,7 +174,7 @@ def plugin_factory(cls) -> type: | |||
if hasattr(cls, "get_render_template") | |||
else {} | |||
), | |||
"__module__": cls.__module__, | |||
"__module__": "djangocms_frontend.cms_plugins", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Hardcoding module may affect debugging and serialization
Overriding module can break stack traces, pickling, and introspection. Either keep the original module or use qualname to group classes.
"__module__": "djangocms_frontend.cms_plugins", | |
"__module__": cls.__module__, |
To simplify the configuration used for component plug-ins,
CMS_COMPONENT_PLUGINS
now accepts entries with a plug-in name in addition to a dotted path. It will look up the plug-in on the plug-in pool.Tests are coming up.
Summary by Sourcery
Simplify component plugin configuration to accept plugin names or dotted paths, introduce resolution logic, prevent duplicate registrations, and update tests and documentation accordingly.
New Features:
Enhancements:
Documentation:
Tests: